fix: convert active rate to fraction to calculate opposite amount #5322
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
| ) | ||
| const activeRateAsFraction = activeRate instanceof Price ? FractionUtils.fromPrice(activeRate) : activeRate |
There was a problem hiding this comment.
The definition of Price is:
export declare class Price<TBase extends Currency, TQuote extends Currency> extends Fraction {
So a price is a valid fraction, why do we need specific handling?
Would be the problem that we use in the math the quotient instead of multiplying by numerator and dividing by denominator for the INPUT case, and the inverse of that for the OUTPUT case
There was a problem hiding this comment.
I see.
I yout test, you multiply 1000000 weis of Ether (1e-12 Ether) by the price of ether --> 3208940687 weis of USDC for 1e18 weis of WETH --> 3208.940687 USDC/ETH
If you multiply you get 3208940687000000 / 1e18 ---> 0.003208940687
Because JSBI is a BigInt library, it will return 0 when you try to divide the numerator and denominator (quotient)
So maybe the issue, is we "expect" the price to be in units and it's in wei, but this is not how it stores the information inside. However, I believe it does the conversion you seek inside the Price implementation, IF ONLY it was a public method https://github.com/Uniswap/sdk-core/blob/main/src/entities/fractions/price.ts#L77
There was a problem hiding this comment.
As far as I can tell by their implementation of the Price, its correct. It makes sure you multiply using the right units. So here when you multiply weiWETH value for a price in weiUSD/weiWETH, you naturally get weiUSDC.
So I think multiplying directly should be actually safer. You just need to not use later the quotient without adjusting the units
Basically the function I share above (adjustedForDecimals) I think should convert a fraction with units of weiUSD/weiWETH into USD/WETH
| expect(value.numerator.toString()).toBe('1000000') | ||
| expect(value.denominator.toString()).toBe('1') | ||
|
|
||
| expect(price.numerator.toString()).toBe('3208940687') | ||
| expect(price.denominator.toString()).toBe('1000000000000000000') |
There was a problem hiding this comment.
That's testing the Uniswap lib methods
There was a problem hiding this comment.
Yeah, I know :)
This is a demo in order to answer on Anxos question
| /** | ||
| * This is still valid fraction, and it does make sense, but let's see what we get bellow | ||
| */ | ||
| expect(multiplied.toFixed(10)).toBe('0.0032089407') |
There was a problem hiding this comment.
That's true that it doesn't work as expected because the Price instances are decimals aware.
That's why we shouldn't use multiply or prices as if they are regular fractions. We should use quote instead for them https://docs.uniswap.org/sdk/core/reference/classes/Price#quote
Summary
Fixes #5310
Sometimes
activeRateis aPriceand because of that output amount (for sell order) calculates wrongly.I don't know why it is Price, in the most of cases it is a
Fraction.To Test